-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Miscellaneous cleanup #14
Conversation
khawkins
commented
Jun 6, 2024
- Renamed some files to follow naming conventions.
- Consolidated some helper functions.
- Updated rule messages and descriptions.
- Renamed some files to follow naming conventions. - Consolidated some helper functions. - Updated rule messages and descriptions.
Everything looks pretty good over all. I did a few cleanup things as mentioned in the description. I'll go through and highlight a couple in the details. |
@@ -0,0 +1,8 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Figured we may as well recommend the ESLint and Pretter extensions, since we make use of both.
@@ -0,0 +1,3 @@ | |||
{ | |||
"editor.defaultFormatter": "esbenp.prettier-vscode" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this just so any potential formatter conflict configured locally wouldn't wreak havoc when formatting in VSCode. Prettier will automatically read its configuration from its standard locations in the project, so we don't need to configure it here, more than just saying that this is the formatter we use.
@@ -14,7 +14,7 @@ import { NO_SEMI_ANTI_JOIN_SUPPORTED_RULE_ID } from '../rules/graphql/no-semi-an | |||
import { NO_MORE_THAN_1_PARENT_RECORD_RULE_ID } from '../rules/graphql/no-more-than-1-parent-record.js'; | |||
import { NO_MORE_THAN_3_CHILD_ENTITIES_RULE_ID } from '../rules/graphql/no-more-than-3-child-entities.js'; | |||
import { NO_MORE_THAN_3_ROOT_ENTITIES_RULE_ID } from '../rules/graphql/no-more-than-3-root-entities.js'; | |||
import { createScopedModuleRuleName } from '../util/createScopedModuleRuleName.js'; | |||
import { createScopedModuleRuleName } from '../util/rule-helpers.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I consolidated these into an aggregate helper utility module, just because it didn't seem like the ones I consolidated, needed to have their own separate space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea moving createRule, createScopedModuleRuleName into graphql-ast-utils.ts.
import { getClosestAncestorByType } from '../../util/graphqlAstUtils'; | ||
import { GraphQLESTreeNode } from './types'; | ||
import { getDocUrl } from '../../util/rule-helpers'; | ||
import { getClosestAncestorByType } from '../../util/graphql-ast-utils'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also renamed a couple of modules/files like this, just for more consistent TypeScript naming conventions.
import { GraphQLESTreeNode } from './types'; | ||
import { getDocUrl } from '../../util/rule-helpers'; | ||
import { getClosestAncestorByType } from '../../util/graphql-ast-utils'; | ||
import { GraphQLESTreeNode } from '../../util/types'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved everything that wasn't explicitly a rule, out of the rules/
tree. It's good to keep the rules tree dedicated to rules, so you can see them all at a glance.
@@ -1,3 +1,27 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be safe, I added the full copy/paste of the license as worded in the original project. I want to make sure we give proper attribution.
I think there's a good chance that if we can properly tweak our project configuration, we'll be able to remove this file in favor of referencing it from the original project. 🤞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are baffled when seeing mousing over shows the type correctly, but has wiggling line. will spend more fiddling around project setting to see if we can cure this dark spot.
test/rules/apex/apex-import.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: should we rename this file apex-import.ts to apex-import.spec.ts to keep the name consistent with other tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a good idea. I'll rename it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great improvement!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good